Skip to content

test: add comprehensive unit tests for model types#490

Closed
ByteBaker wants to merge 1 commit intomodelcontextprotocol:mainfrom
ByteBaker:main
Closed

test: add comprehensive unit tests for model types#490
ByteBaker wants to merge 1 commit intomodelcontextprotocol:mainfrom
ByteBaker:main

Conversation

@ByteBaker
Copy link
Contributor

@ByteBaker ByteBaker commented Oct 18, 2025

Add 112+ non-async unit tests across model modules to increase coverage:

  • content.rs: 34 tests for RawContent variants, type discrimination, meta preservation
  • prompt.rs: 24 tests for Prompt, PromptArgument, role handling, meta behavior
  • resource.rs: 21 tests for Resource, ResourceTemplate, URI schemes, equality
  • tool.rs: 33 tests for Tool, ToolAnnotations, schema handling, default behaviors

Motivation and Context

Better coverage.

How Has This Been Tested?

Yes. All unit tests pass.

Breaking Changes

None.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

@ByteBaker
test: add comprehensive unit tests for model types
5d9d5f7 
Add 112+ non-async unit tests across model modules to increase coverage:
- content.rs: 34 tests for RawContent variants, type discrimination, meta preservation
- prompt.rs: 24 tests for Prompt, PromptArgument, role handling, meta behavior
- resource.rs: 21 tests for Resource, ResourceTemplate, URI schemes, equality
- tool.rs: 33 tests for Tool, ToolAnnotations, schema handling, default behaviors
@github-actions github-actions bot added T-core Core library changes T-model Model/data structure changes labels Oct 18, 2025
@alexhancock
Copy link
Collaborator

alexhancock commented Nov 5, 2025

I appreciate the contribution to improve testing! Thank you

I think we should reconsider some of them however. The value of a test is how many bugs it catches over its lifetime minus the number of times you have to change it when making routine code changes.

Asserting for the existence of properties that are set on a default instance of a class like this:

    #[test]
    fn test_prompt_new() {
        let prompt = Prompt::new("test_prompt", Some("A test prompt"), None);
        assert_eq!(prompt.name, "test_prompt");
        assert_eq!(prompt.description, Some("A test prompt".to_string()));
        assert_eq!(prompt.arguments, None);
        assert_eq!(prompt.title, None);
        assert_eq!(prompt.icons, None);
    }

feels like it will require us to make many test changes when we change models, but not necessarily catch bugs.

What do you think about a revision where we keep only the tests you've added here which rely on some processing internal to the model, and not just asserting on the existence of certain keys etc?

@alexhancock
Copy link
Collaborator

alexhancock commented Jan 12, 2026

Going to close this out for now, but feel free to post thoughts in response to my last message in Nov and re-open with a new viewpoint on the tests if you like!

#490 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jokemanfire jokemanfire jokemanfire approved these changes

+1 more reviewer

@scutuatua-crypto scutuatua-crypto scutuatua-crypto approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

T-core Core library changes T-model Model/data structure changes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ByteBaker @alexhancock @jokemanfire @scutuatua-crypto